-
Notifications
You must be signed in to change notification settings - Fork 330
Update tests to align with mutation testing runs #1361
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: forks/osaka
Are you sure you want to change the base?
Update tests to align with mutation testing runs #1361
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good aside from the locking question. Might want to wait for #1360 to be merged though, since this one looks a bit easier to rebase.
tests/conftest.py
Outdated
lock_file = session.stash[fixture_lock] | ||
session.stash[fixture_lock] = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to avoid locking when running under mutmut?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, but only when fixtures already exist downloaded - we could skip locking then.
Since mutmut has multiple phases, the fixtures exist after the first stat collection
run (this phase happens before the mutation tests start parallely)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't another test run delete the fixtures folder while it's running if we don't lock?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah makes sense - is there a concern given usually the fixtures persist?
I was thinking since mutmut runs parallely - acquiring the lock for each of these process might lead to sequential startup for each of the mutation run? (which should cost some time)
But its true that if somehow fixtures are deleted, then the current workers at that time would fail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose it depends how mutmut runs in parallel. If it's entirely separate processes, without this lock, each one would blow away the fixtures directory when it starts; but with the lock it'll be serialized.
If mutmut works like xdist, then it'll still run in parallel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In mutmut a separate pytest process is launched for each mutation, so these run independently in parallel (i think unlike xdist?) Also, from all my runs i see the fixtures persist, should we take a tradeoff then over a rare chance of deletion? Here's a CI test run - https://github.com/souradeep-das/execution-specs/actions/runs/17051785519
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See here. While the tarball is being extracted, the state on disk is uncertain.
I think we have three options: (1) have each process use its own fixtures directory; (2) use an exclusive lock during extraction, a shared lock during test runs, and write the currently extracted tests to disk somewhere; or (3) live with serialized tests.
I'd pretty strongly prefer not ignoring this issue, because (unless I'm misunderstanding) we might end up in the situation where a mutmut process only sees 10% of the tests and then reports a false positive.
3720b8e
to
6a9c5a6
Compare
tests/json_infra/conftest.py
Outdated
if all_fixtures_ready: | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This exits the context manager and, as I far understand portalocker, releases the lock. We need to hold the shared lock for the duration of the test run to prevent another process coming in and taking the exclusive lock.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh yess, updated to hold the shared lock entirely now. used a while true loop before, that was confusing so removed it!
also tested with deleting fixtures when running mutmut
What was wrong?
associated with #1145
How was it fixed?
angry_mutant
for tests that cannot be run in the mutation testing suiteCorresponding update on mutmut run - souradeep-das/mutmut@cc99e50
mutmut has to be run with a
--pytest-extra-args
option.For instance-
Cute Animal Picture